Skip to content

fix(pr-management-triage): dedupe paginated PRs - #1071

Merged
potiuk merged 1 commit into
apache:mainfrom
awishere:codex/fix-triage-pagination-dedup
Aug 17, 2026
Merged

fix(pr-management-triage): dedupe paginated PRs#1071
potiuk merged 1 commit into
apache:mainfrom
awishere:codex/fix-triage-pagination-dedup

Conversation

@awishere

@awishere awishere commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Deduplicate PRs by number after the full pagination loop.
  • Keep the final occurrence so an updated PR uses its freshest snapshot and correct position.
  • Add regression coverage for a PR moving between pages and for an already-unique result set.

Type of change

  • Skill change (skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package
  • Documentation
  • CI / dev loop

Test plan

  • prek run --all-files passes
  • All 35 pr-management-triage eval fixtures assemble successfully
  • Both pagination-dedup cases pass in exact automated mode
  • All 150 skill-eval framework tests pass
  • Regression fixtures are included

RFC-AI-0004 compliance

  • Vendor neutrality — no provider-specific behavior added

Linked issues

Closes #77

Generative AI disclosure

Codex was used while preparing this change. The contributor reviewed the diff and test results before submission.

Deduplicate the fully fetched list by PR number while retaining each PR’s freshest occurrence and its position in the fetched ordering.

Generated-by: Codex (GPT-5)

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Correct fix for a real bug, and correct in the details that are
easy to get wrong — so it's worth saying which ones I checked rather than
just "LGTM".

The contradiction was genuine: fetch-and-batch.md's loop builds a plain
list (all_prs = [] / all_prs.extend(...) / return all_prs) while
SKILL.md claimed the list was "keyed by number". The pseudocode is what
an agent actually follows, so duplicates got through. Making the two agree
is the right resolution.

What I verified:

  • sort:updated-asc is real (fetch-and-batch.md:127, :280, :459),
    which is what makes the invariant's premise sound. With ascending sort an
    updated PR moves later, so pagination can duplicate a PR but never skip
    one. Dedup therefore closes the only hazard this sort direction has. Had
    the search been updated-desc the interesting bug would have been a
    dropped PR, and dedup alone would not have covered it.
  • Keep-last, not keep-first, is the correct choice. The fresher record
    carries the current headRefOid, which the already-triaged detection and
    the rollup checks depend on; keeping the stale copy would risk triaging
    against an outdated head.
  • The ordering is right. Reverse / dedupe / reverse preserves the
    later position, which is what updated-asc semantics call for. Traced
    against case-1: [11old, 12, 13, 11fresh][12, 13, 11fresh],
    matching expected.json exactly.
  • The new suite assembles. I ran the runner against it: step-config.json
    points at ## Full-pagination loop, which exists verbatim, and the
    extracted system prompt contains both the new pseudocode and the
    invariant. A brand-new suite is where assembly silently breaks, so this
    was worth confirming rather than assuming.
  • The inventory is updated in both places (33 → 35 cases, 3 → 4 steps), and
    the previous 33 was accurate — so the count stays trustworthy.

One bookkeeping note — Closes #77 claims more than this does

#77's acceptance criteria are an in-session set of PR IDs already touched
(any terminal classification), dropping them silently on re-encounter, with
the set living only for the session.

Criteria 1 and 3 are already satisfied by the pre-existing session cache
(fetch-and-batch.md § Session cacheprs.<n>.action_taken /
action_at, and "Discard the entire bundle on session exit"). Criterion 2
is not what this PR does: seen_numbers is local to one fetch pass and
discarded, so a PR already acted on earlier in the session is not
suppressed. This PR dedupes within a fetch — the mechanism #77 offered as
its example, rather than the requirement it stated.

That is not a defect in the change, and I am not asking you to widen it.
There is a real design tension for the maintainers to settle: criterion 2
partly conflicts with the cache's own rule that a head_sha mismatch means
"drop it and re-classify" — an updated PR is deliberately re-read, because
new commits warrant a fresh look. So #77 likely needs re-scoping, not more
code.

Merging closes #77 automatically (the keyword is in the PR description, so
a merge-commit message cannot override it). I will re-open it with this
context attached so the remaining question is not lost.

Thanks — the sort-direction reasoning in the invariant is what made this
quick to review with confidence.


This review was drafted by an AI-assisted tool and
confirmed by a Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Magpie handles maintainer review:
CONTRIBUTING.md.

@potiuk
potiuk merged commit c618b93 into apache:main Aug 17, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Silent in-session de-dup of already-seen / already-skipped PRs

2 participants